audit: fix NUL handling in untrusted strings

From: Miloslav Trmac <[email protected]>

audit_string_contains_control() stops checking at the first NUL byte. If audit_string_contains_control() returns FALSE, audit_log_n_untrustedstring() submits the complete string - including the NUL byte and all following bytes, up to the specified maximum length
- to audit_log_n_string(), which copies the data unchanged into the audit record.

The audit record can thus contain a NUL byte (and some unchecked data after that). Because the user-space audit daemon treats audit records as NUL-terminated strings, an untrusted string that is shorter than the specified maximum length effectively terminates the audit record.

This patch modifies audit_log_n_untrustedstring() to only log the data before the first NUL byte, if any.

---
kernel/audit.c | 21 ++++++++++++++++-----

 1 file changed, 16 insertions(+), 5 deletions(-)

index 4414e93..03b6397 100644
@@ -1362,6 +1362,12 @@ void audit_log_n_string(struct audit_buffer *ab, const char *string,
 	skb_put(skb, slen + 2);	/* don't include null terminator */
 }
 
+static inline int
+audit_is_control_character(unsigned char c)
+{
+	return c == '"' || c < 0x21 || c > 0x7E;
+}
+
 /**
  * audit_string_contains_control - does a string need to be logged in hex
  * @string: string to be checked

@@ -1371,7 +1377,7 @@ int audit_string_contains_control(const char *string, size_t len)
 {
 	const unsigned char *p;
 	for (p = string; p < (const unsigned char *)string + len && *p; p++) {
-		if (*p == '"' || *p < 0x21 || *p > 0x7e)
+		if (audit_is_control_character(*p))
 			return 1;
 	}
 	return 0;

@@ -1394,10 +1400,15 @@ int audit_string_contains_control(const char *string, size_t len)
 void audit_log_n_untrustedstring(struct audit_buffer *ab, const char *string,
 				 size_t len)
 {
-	if (audit_string_contains_control(string, len))
-		audit_log_n_hex(ab, string, len);
-	else
-		audit_log_n_string(ab, string, len);
+	const unsigned char *p;
+
+	for (p = string; p < (const unsigned char *)string + len && *p; p++) {
+		if (audit_is_control_character(*p)) {
+			audit_log_n_hex(ab, string, len);
+			return;
+		}
+	}
+	audit_log_n_string(ab, string, p - (const unsigned char *)string);
 }
 
 /**

I'm going to have to say NAK on this patch.

It's still not right looking at the other user, audit_log_single_execve_arg(). An execve arg with a NULL could loose the stuff after the NULL (not break the record like audit_tty) since the execve uses %s rather than calling trusted string.

How about we change the meaning of audit_string_contains_control() return values? If it returns positive that is the number of bytes in a legitimate string up to the first null. -1 means it is hex. That eliminates your code duplication and allows us to do the right thing in both the generic untrusted_string code and the execve code.....

-Eric

Thanks for the review.
Eric Paris píše v Čt 11. 09. 2008 v 10:25 -0400:

execve() arguments are NUL-terminated strings:
audit_log_single_execve_arg() starts with

	len_left = len = strnlen_user(p, MAX_ARG_STRLEN) - 1;

and the two cycles handle chunks in the first "len" bytes of "p". audit_log_single_execve_arg() never touches any bytes after the first NUL.

So, when audit_log_single_execve_arg() calls audit_string_contains_control(buf, to_send), strlen(buf) == to_send and the patch does not change anything.

That is possible, although the return value convention is somewhat complex. Anyway, no change to the semantics of audit_string_contains_control() is necessary for execve() argument logging.
Mirek

I agree with Eric, this is the wrong solution, but for different (additional) reasons.

It's incumbent upon the kernel audit system to correctly log all string data and not try to interpret the contents of that string data. Special processing with regards to the presence or absence of a null byte is one example of prohibited interpretation. An attacker could hide information after a null byte if it knew everything after the null byte was being discarded. It is critical for post-mortem analysis to be able to reconstruct the fact string data was passed somewhere which contained a null byte which may have been a trigger for subsequent behaviour. In addition depending on the string data character encoding it is possible to have legitimate null bytes which do not represent string termination. Making assumptions about the character encoding of an octet sequence is another example of prohibited interpretation. In the particular example cited the string data is actually part of terminal protocol which in fact permits nulls because it's not string data but rather an octet sequence.

The important point is:
A string value should always be a counted octet sequence for the purpose of auditing.

It seems to me the problem is with audit_string_contains_control():

int audit_string_contains_control(const char *string, size_t len) {
const unsigned char *p;

   for (p = string; p < (const unsigned char *)string + len && *p; p++) {
       if (*p == '"' || *p < 0x21 || *p > 0x7e)
           return 1;

}
return 0;
}

The problem is that it is passed a counted octet sequence but in some circumstances ignores the count. This occurs when *p == 0, the test for NULL should be removed. If that test is removed it will return the flag which indicates the string must be encoded differently to be conformant with the protocol.

With this change auditd will not terminate the record prematurely because the string value will have been properly encoded according to the protocol.

As a side note I'm concerned there may be places in the user audit code which treat string data as null terminated (at least that is my recollection). If so that is an area which needs to be fixed to treat string values as counted octet sequences.

John Dennis <[email protected]>

John Dennis píše v Čt 11. 09. 2008 v 13:30 -0400:

This is UNIX, "string" means "NUL-terminated string" (in fact the presence of a NUL byte is the only way to reasonably detect binary data).

You're far more likely to encounter a fixed-length field with an optional terminating NUL (like the old-style, 16-byte directory entries) than an ASCII-compatible string that intentionally contains a NUL byte.

TTY input auditing was the only place where it makes a difference, all other code was passing a string that was at least as long as the specified size to audit_log_n_untrustedstring().

Yes, that's possible - but then audit_log_n_untrustedstring() would be more accurately called audit_log_n_ascii_like_binary_data().

Anyway, Eric/Al - if you prefer this solution, I can prepare an alternative patch.

Yes, auditd adds a NUL terminator to the audit record, and then treats it as a regular NUL-terminated string; if the audit record contains an embedded NUL byte, the rest of the record is discarded by auditd.
Mirek

Miloslav Trmač píše v Čt 11. 09. 2008 v 20:10 +0200:

... my original patch (which stops at the first NUL) works the same way the other interfaces with maximum string length (e.g. strnlen() or printf ("%.5s", ...)) do.
Mirek

In every case where this occurs (kernel or user space), the field values are expected to be encoded to prevent it from being discarded.

-Steve

This is true. The proposed patch defeats the encoding of the entire data block and thus fails the criteria Steve correctly states is a requirement.

The concern I have in the user level audit code is not with handling the encoded string values which is fine, but rather with the handling the decoded string block.

John Dennis <[email protected]>

A primary purpose of the audit system is to log with the greatest accuracy possible the actual data. If that data somehow contained a null, even in a context in which a null would have been prohibited, the audit system absolutely needs to be able to correctly record that aberrant event and it's actual data. If the audit system failed at that moment it's failing at the worst possible moment, the moment when you're looking for bad data.

A UNIX-like operating system does not in and of itself mandate the default conventions of the C programming language. A great danger and the source of bugs is making assumptions concerning how to interpret octet sequences. By far and away the worst possible place to make a mistake handling an octet sequence is in the kernel. It would be wrong for the audit system to assume the memory block it was pointed to only ever contained null terminated ascii strings, especially when the memory block is terminated by virtue of an octet count.

John Dennis <[email protected]>

John Dennis píše v Čt 11. 09. 2008 v 15:12 -0400:

If the interface says "NUL-terminated string", any bytes after that are not "actual data". The bytes might be useful for diagnosing an anomaly if the kernel's behavior somehow depended on the bytes after NUL due to a kernel bug - but the kernel's behavior might depend on anything due to such a bug, and we don't log the complete state of the system in each audit message. The "actual data" of a string is only up to the NUL byte.

Yes, that's why it was wrong to use audit_*string() for TTY input data. And the 2/2 patch fixes it - at the source of the problem, not in an unrelated function that was incorrectly used.
Mirek

Yes, that's correct. However, the function in question, audit_log_n_untrustedstring() is not an interface accepting a null terminated string, it accepts a count. The helper function on which it is dependent, audit_string_contains_control(), disregards the length parameter it is passed and thus audit_log_n_untrustedstring() misbehaves as a consequence.

This is true, but it's only part of the problem, the string functions still need to be robust, even used inappropriately.

John Dennis <[email protected]>

John Dennis píše v Čt 11. 09. 2008 v 15:47 -0400:

These options are not exclusive - see strnlen(). audit_log_n_untrustedstring(), as patched, follows that model exactly.

They need to do what they promise in the first place, only then can they possibly do something useful with invalid input. AUDIT_USER_TTY messages, like all messages sent from user-space, are sent with a trailing NUL. The contents of the message are (more or less) trusted, it is perfectly reasonable to stop at first NUL. audit_log_n_untrustedstring(), as patched, does exactly what is needed - the length parameter is used only to make unrelated kernel data does not appear in the audit record.

Now, given a function called fn(), either behavior (terminating at NUL, or treating NUL as a control character) is valid, and AUDIT_USER_TTY handling can be written for both behaviors of fn(). But given the function is called ...untrustedstring, and "string" in C implies NUL-terminated, I think it is more consistent to stop on NUL.
Mirek

It's unclear how serious this problem is. Do you believe that it is sufficiently serious to warrant merging these fixes into 2.6.27? 2.6.26.x? 2.6.25.x?

Thanks.

Andrew Morton píše v Čt 11. 09. 2008 v 12:14 -0700:

* AUDIT_USER_TTY records (which are sent from user-space with a trailing NUL byte) are missing a terminating '"' character.
* Some data is not recorded in AUDIT_TTY records, and the terminating '"' character is missing in that case as well.

This patch (1/2) only fixes creation of incorrectly formatted, but easy-to-understand audit records; it would be nice to have it in 2.6.27 (assuming the audit maintainer acks it - or a variant of it). The other one (2/2), which makes sure all TTY audit data is recorded, should probably be merged in the stable releases as well.
Mirek